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

[brianlball]

Fixes #841.

Problem

During the July 2026 production incident, corrupt seed.zip uploads produced thousands of failed ResqueJobs::InitializeAnalysis jobs, all with "zlib error while inflating". A corrupt zip was accepted at upload, extraction was retried 3 times (a corrupt file never becomes valid on retry), and when the job finally failed the analysis was left in queued/started forever with no API-visible failure state and no cleanup — RunAnalysis is only enqueued from an after_perform hook, which Resque skips when perform raises.

One correction to the issue text: a failed Resque job does not block the queue (failed jobs go to the failed list and the worker moves on), and there is no Resque-level retry configured — the "3 times" in the error message is an inline retry loop in Analysis#run_initialization. So one corrupt upload produces one failed job, and 2641 failed jobs implies roughly that many submissions (likely a client resubmitting). The incident data is worth checking for whether the failed queue held one analysis id or many. The fix targets the real damage: analyses stranded in a non-terminal state, futile retries, and no rejection at upload time.

Changes

Reject corrupt zips at upload (422). New Analysis.seed_zip_error(path) validates zip structure and inflates every entry to verify CRCs. AnalysesController#upload and #create return 422 with a descriptive error_message instead of accepting a corrupt file. Validation is strictly read-only: it deliberately avoids the Zip::File.open block form, whose implicit close can commit and rewrite the archive being read.

Fail fast on corrupt zips during initialization. Analysis#run_initialization now rescues Zip::Error/Zlib::Error separately: no retries (the failure is deterministic), the partially extracted directory is removed so a later re-run cannot skip-and-reuse stale files, and the error names the analysis. Transient errors keep the existing 3-attempt retry.

Terminal, API-visible failure state. InitializeAnalysis.perform now rescues, calls the new Analysis#fail_job! (sets the job status to failed plus a status message), and re-raises so Resque still records the failed job. Analyses no longer sit in queued forever; /analyses/:id/status.json reports failed. failed is a new job-status string; Analysis.status_states is not referenced by any live code path.

Client impact (PAT / OSAF / openstudio-analysis gem)

• Upload rejection: PAT submits via openstudio_meta run_analysis → OpenStudio::Analysis::ServerApi#new_analysis, which raises on any non-201 upload response, so the meta CLI exits nonzero and PAT reports the failure at submission time, with the server's error_message in PAT's console log. Scripted OSAF use of the gem gets the same immediate raise.
• failed status: PAT's run screen stops polling only on completed and otherwise displays the raw status string. A failed analysis therefore displays as "failed" while polling continues until the user stops it — the same behavior as today's eternally-"queued" stuck analysis, but now the user can see it failed and why (status_message). No string comparison in PAT breaks on the new value. A reasonable PAT follow-up is to also stop polling on failed.
• OSAF proper does not poll analysis status. openstudio_meta run_analysis is fire-and-forget, and the gem's remaining status checks are datapoint-level (dp[:status] == 'completed' for report downloads), which this change does not touch.

Testing

• New specs: server/spec/models/analysis_init_spec.rb (9 examples: validator accept/reject including a CRC-mismatch archive that opens and extracts cleanly, fail-fast with no retry plus partial-extraction cleanup, transient-error retry preserved, fail_job!, and InitializeAnalysis.perform failure handling) and server/spec/requests/analyses_upload_spec.rb (3 examples: 422 for garbage and truncated uploads with descriptive errors, 201 for a valid zip). Red-green verified: with the app changes reverted, 10 of 12 fail (the transient-retry example intentionally passes on the old code).
• All 12 pass in a local replica of the CI docker environment (nrel/openstudio-server:develop image, mongo as db, redis as queue, RAILS_ENV=docker), and the neighboring existing specs (analysis_spec, request analyses_spec) still pass.
• CI already runs these specs implicitly: the linux/macos jobs execute the full server suite through openstudio_meta run_rspec, which reproduces the PAT local deployment (delayed_job, local mongo) and writes rspec output to spec/unit-test/logs/rspec.log rather than the console — easy to miss when reading CI logs. Both jobs were green on this branch with the new specs included.
• The docker job, however, has not run server model/request specs since Sept 2020 (49580920) — and Enhance InitializeAnalysis resilience for corrupt seed.zip files #841 is specific to the resque path, which only the docker environment exercises. This PR adds the two spec files to run-server-tests.sh so the resque/redis/authenticated-mongo environment runs them too (about 1s, before the feature specs, leaves the db empty).
• Note for Windows developers: the "accepts a valid seed zip" request spec fails on Windows only, because rack-test 2.2.0 builds multipart bodies with a text-mode read that truncates binary files at the first 0x1A byte. It passes on Linux/macOS; the validator's acceptance of that exact fixture is also covered directly by a model spec.

Deferred (from the issue's proposals)

• SHA-256 at upload + re-verify before extraction (solution 4): complementary at-rest integrity check, straightforward follow-up.
• Rake task / migration for analyses already stuck from past corrupt uploads.
• Dead-letter queue (solution 3): not needed — failed Resque jobs already sit in the failed list without blocking the queue; the missing piece was the terminal analysis state, which this PR adds.

🤖 Generated with Claude Code

[brianlball]

The first docker-job run of the new CI line failed — root-caused and fixed in 3721a72:

The 12 new specs themselves passed in the docker (resque) environment, but they broke the feature specs that run after them. docker-compose exec runs rspec as root while the live app serves as nobody; the specs' paperclip writes left /mnt/openstudio/server/assets/analyses root-owned, so every subsequent upload from the live app returned Errno::EACCES → HTTP 500 (custom_gems and 9/10 algo failures), and a leftover factory project broke the empty-projects assertion in test_apis (its created_at matched the spec run to the second).

Fix: after(:all) { Project.destroy_all } in both spec files — paperclip deletes the attachments and prunes the emptied root-owned directory, so the live app recreates it under its own user. Verified in a fresh local replica of the CI docker environment in CI order: 12 examples 0 failures, directory pruned, projects list empty, then a valid upload returned 201 (directory recreated by nobody) and a corrupt upload returned 422 through the live nginx/Passenger stack — which also serves as an end-to-end check of the #841 validation itself.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR addresses incident #841 by preventing corrupt seed.zip files from entering the analysis pipeline, avoiding futile initialization retries on deterministic ZIP/Zlib failures, and ensuring analyses reach an API-visible terminal failed state when initialization fails (while still letting Resque record the job as failed).

Changes:

• Add deep ZIP validation (Analysis.seed_zip_error) and return 422 Unprocessable Entity during create/upload when a corrupt/truncated/invalid ZIP is detected.
• Fail fast on Zip::Error / Zlib::Error during initialization (no retry) and clean up partial extraction directories.
• Add Resque job-level rescue handling to mark the analysis/job as failed before re-raising, plus add targeted specs and run them in the docker server test script.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
server/spec/requests/analyses_upload_spec.rb	Adds request coverage for 422 rejection of corrupt/truncated uploads and 201 acceptance of valid ZIPs.
server/spec/models/analysis_init_spec.rb	Adds model/job specs for ZIP validation, fail-fast extraction behavior, cleanup, and terminal failure state propagation.
server/app/models/analysis.rb	Introduces seed_zip_error, fail_job!, and fail-fast ZIP/Zlib handling in run_initialization.
server/app/jobs/resque_jobs/initialize_analysis.rb	Marks analysis/job failed on initialization exceptions, then re-raises so Resque records failure.
server/app/controllers/analyses_controller.rb	Validates uploaded/created seed ZIPs and returns 422 with a descriptive error_message on corruption.
docker/server/run-server-tests.sh	Ensures the new model/request specs run in the docker (Resque/Redis/Mongo) CI path.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[brianlball]

Good catch that this path was broken, though the failure mode is different: there is no new.html.erb view in the app at all (the HTML create path is dead code — analyses are created via the JSON API), so render action: 'new' raises ActionView::MissingTemplate regardless of @analysis. The pre-existing error branch below has the same landmine. Fixed in a8c8855 by rendering a plain-text 422 instead of a template.

[brianlball]

Agreed — addressed in a8c8855 with streaming caps counted inside the read loop: SEED_ZIP_MAX_INFLATED_BYTES (10 GB) and SEED_ZIP_MAX_ENTRIES (100k), each returning a descriptive 422-able error when exceeded. No Timeout needed since we control the loop, and no async validation — deferring the check would reintroduce exactly the delayed-failure problem #841 is about. Covered by two new specs using stub_const with tiny caps. Worth noting the pre-existing extraction path in run_initialization still inflates unbounded zips (with a 1-hour timeout) on the web node; bounding that is a separate concern from this PR.