Fail fast on corrupt seed.zip: validate at upload, no futile retries, terminal 'failed' state (#841)

docker/server/run-server-tests.sh

@@ -19,6 +19,9 @@ do
 done
 
 #cd /opt/openstudio/server && bundle exec rspec; (( 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 || $? ))
 # 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/app/controllers/analyses_controller.rb

@@ -140,6 +140,21 @@ def create
     params = analysis_params
     params[:project_id] = project_id
 
+    # Reject corrupt seed zips at creation time with a 422 instead of letting them fail
+    # later in ResqueJobs::InitializeAnalysis and strand the analysis (issue #841)
+    if params[:seed_zip].respond_to?(:path)
+      zip_error = Analysis.seed_zip_error(params[:seed_zip].path)
+      if zip_error
+        respond_to do |format|
+          # no new.html.erb view exists (analyses are created via the JSON API), so
+          # render plain text rather than a missing template
+          format.html { render plain: zip_error, status: :unprocessable_entity }
+          format.json { render json: { status: 'error', error_message: zip_error }, status: :unprocessable_entity }
+        end
+        return
+      end
+    end
+
     @analysis = Analysis.new(params)
     @analysis.save! # Make sure to save it before processing it further. Rails 5 upgrade issue.
 
@@ -391,6 +406,15 @@ def upload
     @analysis = Analysis.find(params[:id])
 
     if @analysis
+      # Reject corrupt seed zips at upload time with a 422 instead of letting them fail
+      # later in ResqueJobs::InitializeAnalysis and strand the analysis (issue #841)
+      if params[:file].respond_to?(:path)
+        zip_error = Analysis.seed_zip_error(params[:file].path)
+        if zip_error
+          render json: { status: 'error', error_message: zip_error }, status: :unprocessable_entity
+          return
+        end
+      end
       @analysis.seed_zip = params[:file]
     end
 

server/app/jobs/resque_jobs/initialize_analysis.rb

@@ -9,13 +9,21 @@ class InitializeAnalysis
 
     # Perform set up before running an analysis
     # this is enqueued in Analysis#start.
-    # todo error handling
-    # todo handle cleanup if this fails
     def self.perform(analysis_type, analysis_id, job_id, options = {})
       # TODO: error handling and logging around looking up analysis and detecting start/complete
       analysis = Analysis.find(analysis_id)
       # this will handle unzipping to osdata volume and running any initialization scripts
       analysis.run_initialization
+    rescue StandardError => e
+      # Mark the analysis failed so it reaches a terminal state visible to clients instead
+      # of sitting in 'queued' forever, then re-raise so Resque records the failed job (issue #841).
+      Rails.logger.error "InitializeAnalysis failed for analysis #{analysis_id}: #{e.message}"
+      begin
+        analysis&.fail_job!("Analysis initialization failed: #{e.message}")
+      rescue StandardError => mark_error
+        Rails.logger.error "Could not mark analysis #{analysis_id} as failed: #{mark_error.message}"
+      end
+      raise
     end
 
     # after_perform hooks only called if job completes successfully

server/app/models/analysis.rb

@@ -93,6 +93,59 @@ def self.status_states
     ['na', 'init', 'queued', 'started', 'post-processing', 'completed']
   end
 
+  # Caps for upload-time zip validation. Far above any real seed zip, but fatal to a
+  # zip bomb that would otherwise pin a web worker inflating unbounded data in the
+  # request path.
+  SEED_ZIP_MAX_INFLATED_BYTES = 10 * 1024 * 1024 * 1024 # 10 GB
+  SEED_ZIP_MAX_ENTRIES = 100_000
+
+  # Validate that a file is a structurally sound ZIP whose entries inflate cleanly with
+  # matching CRCs. Returns nil if valid, otherwise a String describing the problem.
+  # Called at upload time so corrupt seed zips are rejected with a 422 instead of
+  # failing later in ResqueJobs::InitializeAnalysis and stranding the analysis (issue #841).
+  def self.seed_zip_error(file_path)
+    # Zip::File.new instead of the Zip::File.open block form: open's implicit close
+    # calls commit, which can REWRITE the archive being validated. new reads the
+    # central directory and releases the file handle, keeping validation read-only.
+    entries = 0
+    inflated_bytes = 0
+    zf = ::Zip::File.new(file_path)
+    zf.each do |entry|
+      next unless entry.file?
+
+      entries += 1
+      return "seed zip contains more than #{SEED_ZIP_MAX_ENTRIES} file entries" if entries > SEED_ZIP_MAX_ENTRIES
+
+      crc = ::Zlib.crc32
+      entry.get_input_stream do |io|
+        while (chunk = io.read(1_048_576))
+          inflated_bytes += chunk.bytesize
+          return "seed zip inflates to more than #{SEED_ZIP_MAX_INFLATED_BYTES} bytes" if inflated_bytes > SEED_ZIP_MAX_INFLATED_BYTES
+
+          crc = ::Zlib.crc32(chunk, crc)
+        end
+      end
+      return "seed zip entry '#{entry.name}' is corrupt (CRC mismatch)" unless crc == entry.crc
+    end
+    nil
+  rescue ::Zip::Error, ::Zlib::Error => e
+    "seed zip is not a valid ZIP archive: #{e.message}"
+  end
+
+  # Mark the analysis's most recent job as failed so the analysis reaches a terminal,
+  # API-visible state instead of sitting in 'queued'/'started' forever when
+  # initialization fails (issue #841).
+  def fail_job!(message)
+    self.status_message = message
+    save!
+    job = jobs.order_by(:index.asc).last
+    return unless job
+
+    job.status = 'failed'
+    job.status_message = message
+    job.save!
+  end
+
   # FIXME: analysis_type is somewhat ambiguous here, as it's argument to this method and also a class method name
   def start(no_delay, analysis_type = 'batch_run', options = {})
     Rails.logger.debug "analysis.start enter"
@@ -495,6 +548,11 @@ def run_initialization
         # OpenStudio::Workflow.extract_archive(download_file, analysis_dir)
         extract_archive(seed_zip.path, shared_directory_path)
       end
+    rescue ::Zip::Error, ::Zlib::Error => e
+      # A corrupt zip is a deterministic failure - retrying cannot succeed (issue #841).
+      # Remove any partially extracted files so a later re-run does not skip-and-reuse them.
+      FileUtils.rm_rf(shared_directory_path)
+      raise "Seed zip for analysis #{id} is corrupt and cannot be extracted: #{e.message}"
     rescue StandardError => e
       retry if extract_count < extract_max_count
       raise "Extraction of the seed.zip file failed #{extract_max_count} times with error #{e.message}"

server/spec/models/analysis_init_spec.rb

@@ -0,0 +1,180 @@
+# *******************************************************************************
+# OpenStudio(R), Copyright (c) Alliance for Sustainable Energy, LLC.
+# See also https://openstudio.net/license
+# *******************************************************************************
+
+require 'rails_helper'
+require 'tmpdir'
+
+# Regression specs for https://github.com/NatLabRockies/OpenStudio-server/issues/841 -
+# a corrupt seed.zip must fail fast (no pointless retries of a deterministic failure),
+# leave the analysis in a terminal 'failed' state visible to API clients, and be
+# detectable before the InitializeAnalysis job ever runs.
+RSpec.describe Analysis, type: :model do
+  before :all do
+    Project.destroy_all
+    @analysis = FactoryBot.create(:analysis)
+    @tmp_dir = Dir.mktmpdir('analysis-init-spec')
+  end
+
+  after :all do
+    # Destroy factory data so paperclip deletes the seed zip files and prunes the
+    # emptied assets/analyses directory. In the docker CI job this spec runs as root
+    # 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
+    FileUtils.rm_rf(@tmp_dir)
+  end
+
+  def build_valid_zip(path)
+    Zip::OutputStream.open(path) do |zos|
+      zos.put_next_entry('data/seed.txt')
+      zos.write 'seed zip payload for crc check'
+    end
+    path
+  end
+
+  # STORED (uncompressed) entry so the byte flip below corrupts the payload without
+  # breaking the zip structure - the archive still opens and extracts, only a CRC
+  # comparison can catch the corruption
+  def build_crc_corrupt_zip(path)
+    Zip::OutputStream.open(path) do |zos|
+      zos.put_next_entry('data/seed.txt', nil, nil, Zip::Entry::STORED)
+      zos.write 'seed zip payload for crc check'
+    end
+    content = File.binread(path)
+    File.binwrite(path, content.sub('seed zip payload', 'SEED ZIP PAYLOAD'))
+    path
+  end
+
+  # Cut the archive in half: the 'PK' magic bytes survive (so content-type checks pass)
+  # but the central directory is gone, which is what a partial/interrupted upload looks like
+  def build_truncated_zip(path)
+    build_valid_zip(path)
+    content = File.binread(path)
+    File.binwrite(path, content[0, content.length / 2])
+    path
+  end
+
+  def attach_truncated_seed_zip(analysis)
+    truncated = build_truncated_zip(File.join(@tmp_dir, 'truncated_seed.zip'))
+    analysis.seed_zip = File.new(truncated)
+    analysis.save!
+    analysis
+  end
+
+  describe '.seed_zip_error' do
+    it 'returns nil for a valid zip' do
+      # Validates: upload validation must not reject healthy seed zips
+      path = build_valid_zip(File.join(@tmp_dir, 'valid.zip'))
+      expect(Analysis.seed_zip_error(path)).to be_nil
+    end
+
+    it 'returns nil for the reference seed zip fixture' do
+      # Validates: the real fixture used across the suite passes validation
+      expect(Analysis.seed_zip_error("#{Rails.root}/spec/files/batch_datapoints/example_csv.zip")).to be_nil
+    end
+
+    it 'reports a file that is not a zip archive' do
+      # Regression: issue #841 - garbage uploads were accepted and only failed in InitializeAnalysis
+      path = File.join(@tmp_dir, 'garbage.zip')
+      File.binwrite(path, 'this is not a zip archive at all')
+      expect(Analysis.seed_zip_error(path)).to match(/not a valid ZIP archive/)
+    end
+
+    it 'reports a truncated zip archive' do
+      # Regression: issue #841 - partially uploaded seed zips were accepted and only failed in InitializeAnalysis
+      path = build_truncated_zip(File.join(@tmp_dir, 'truncated.zip'))
+      expect(Analysis.seed_zip_error(path)).to match(/not a valid ZIP archive/)
+    end
+
+    it 'reports an entry whose payload does not match its CRC' do
+      # Validates: bit-rot that keeps the zip structure intact is still caught (rubyzip
+      # 2.x does not verify CRCs on extract, so this is the only line of defense)
+      path = build_crc_corrupt_zip(File.join(@tmp_dir, 'crc_corrupt.zip'))
+      expect(Analysis.seed_zip_error(path)).to match(/entry 'data\/seed\.txt' is corrupt \(CRC mismatch\)/)
+    end
+
+    it 'rejects an archive that inflates past the size cap' do
+      # Validates: validation must not inflate unbounded data in the request path - a
+      # zip bomb would otherwise pin a web worker (PR #844 review)
+      stub_const('Analysis::SEED_ZIP_MAX_INFLATED_BYTES', 10)
+      path = build_valid_zip(File.join(@tmp_dir, 'inflates_past_cap.zip'))
+      expect(Analysis.seed_zip_error(path)).to eq 'seed zip inflates to more than 10 bytes'
+    end
+
+    it 'rejects an archive with more file entries than the cap' do
+      # Validates: entry-count cap bounds validation work in the request path (PR #844 review)
+      stub_const('Analysis::SEED_ZIP_MAX_ENTRIES', 1)
+      path = File.join(@tmp_dir, 'too_many_entries.zip')
+      Zip::OutputStream.open(path) do |zos|
+        zos.put_next_entry('a.txt')
+        zos.write 'a'
+        zos.put_next_entry('b.txt')
+        zos.write 'b'
+      end
+      expect(Analysis.seed_zip_error(path)).to eq 'seed zip contains more than 1 file entries'
+    end
+  end
+
+  describe '#run_initialization with a corrupt seed zip' do
+    it 'fails on the first attempt without retrying and cleans up partial extraction' do
+      # Regression: issue #841 - corrupt zips were retried 3 times ("Extraction of the
+      # seed.zip file failed 3 times with error zlib error while inflating") even though
+      # the failure is deterministic
+      attach_truncated_seed_zip(@analysis)
+      FileUtils.mkdir_p(@analysis.shared_directory_path)
+      File.write(File.join(@analysis.shared_directory_path, 'partial_file.txt'), 'stale partial extraction')
+
+      expect(@analysis).to receive(:extract_archive).once.and_call_original
+      expect { @analysis.run_initialization }.to raise_error(/corrupt and cannot be extracted/)
+      expect(Dir.exist?(@analysis.shared_directory_path)).to be(false), 'partial extraction dir must be removed so a re-run does not skip-and-reuse stale files'
+    end
+
+    it 'still retries transient extraction errors up to 3 attempts' do
+      # Validates: fail-fast on corrupt zips must not remove the retry that papers over
+      # transient IO failures (e.g. NFS hiccups on the osdata volume)
+      attempts = 0
+      allow(@analysis).to receive(:extract_archive) do
+        attempts += 1
+        raise Errno::EIO, 'transient io failure' if attempts < 3
+      end
+
+      expect { @analysis.run_initialization }.not_to raise_error
+      expect(attempts).to eq(3), 'transient errors should be retried until the 3-attempt cap'
+    end
+  end
+
+  describe '#fail_job!' do
+    it 'marks the newest job failed so the analysis reaches a terminal state' do
+      # Regression: issue #841 - analyses with failed initialization sat in 'queued'
+      # forever with no API-visible failure state
+      job = Job.new_job(@analysis.id, 'batch_run', 0, {})
+
+      @analysis.fail_job!('seed zip is corrupt')
+
+      expect(job.reload.status).to eq 'failed'
+      expect(job.status_message).to eq 'seed zip is corrupt'
+      expect(@analysis.reload.status_message).to eq 'seed zip is corrupt'
+      expect(@analysis.status).to eq 'failed'
+    end
+  end
+
+  describe 'ResqueJobs::InitializeAnalysis.perform with a corrupt seed zip' do
+    it 'marks the analysis failed and re-raises so Resque records the failure' do
+      # Regression: issue #841 - InitializeAnalysis had no error handling; failures left
+      # the analysis stuck and RunAnalysis was silently never enqueued
+      attach_truncated_seed_zip(@analysis)
+      job = Job.new_job(@analysis.id, 'batch_run', 0, {})
+
+      expect do
+        ResqueJobs::InitializeAnalysis.perform('batch_run', @analysis.id, job.id)
+      end.to raise_error(/corrupt and cannot be extracted/)
+
+      expect(job.reload.status).to eq 'failed'
+      expect(job.status_message).to match(/Analysis initialization failed: Seed zip for analysis #{@analysis.id} is corrupt/)
+      expect(@analysis.reload.status).to eq 'failed'
+    end
+  end
+end