Harden RunSimulateDataPoint worker against permanent lock deadlock and stale-job re-dispatch

[anchapin]

Summary

Production incident (2026-07-07/08): 50+ analyses with corrupted seed zips triggered a permanent deadlock in the worker-side lock/receipt coordination in DjJobs::RunSimulateDataPoint, wedging ~9,988 worker pods (99.6% of the worker tier) into an infinite wait. Root-caused and fixed here; not related to (but complementary to) the seed-zip validation fix in #841/#844, which covers a different, once-per-analysis web-node code path (Analysis#run_initialization). This fix targets the per-datapoint, per-worker-pod download+extract path (initialize_worker/write_lock).

Root cause

write_lock released its flock on failure but never deleted the lock file, and never wrote a receipt. The waiter loop in initialize_worker only polled for the receipt file appearing — it never rechecked whether the lock file itself had disappeared (holder crashed) — so every subsequent worker for the same analysis blocked for the full initialize_worker_timeout (8h default), and then, due to a second bug, fell through to return true anyway: reporting fake success and letting perform proceed to simulate against a directory that was never populated.

Changes

• write_lock: rescue any exception in the protected block, delete the lock file before re-raising (previously only released the flock — leaving a stat-able "still locked" file behind). Also closes the file handle in ensure (minor fd leak).
• initialize_worker's receipt-wait loop: detect lock_holder_gone (lock file disappears without a receipt appearing) and return false immediately instead of looping to timeout. On genuine Timeout::Error, explicitly delete the stale lock file and return false instead of the old fall-through return true.
• Zip extraction retry: add rescue ::Zip::Error, ::Zlib::Error ahead of the generic retry rescue — fail fast on deterministic corruption (no futile 3x retry), remove the partial analysis_dir. Worker-side counterpart to Enhance InitializeAnalysis resilience for corrupt seed.zip files #841/Fail fast on corrupt seed.zip: validate at upload, no futile retries, terminal 'failed' state (#841) #844's fail-fast, applied to the different per-datapoint download path.
• @@data_point → @data_point typo fix (5 occurrences: 3 in dj_jobs/run_simulate_data_point.rb, 2 in dj_jobs/urban_opt.rb, which is included into the same class): the invalid-timeout fallback branches referenced an uninitialized class variable instead of the instance variable, since ebc2361b (2019). Confirmed also present on origin/develop. Previously any invalid timeout value raised NameError instead of applying the intended 28800s default.
• resque_jobs/run_simulate_data_point.rb#perform: added a narrow analysis.run_flag == false guard (checked ahead of the existing completed-status guard) to skip stale queue payloads for analyses explicitly stopped/quarantined via stop_analysis/soft_stop_analysis. Deliberately not a blanket status == 'completed' check — that would break the admin manual-retry workflow, since requeue/requeue_started don't reset status before re-enqueueing.

Testing

Added server/spec/models/dj_run_simulate_data_point_hardening_spec.rb (7 examples) and server/spec/models/resque_run_simulate_data_point_hardening_spec.rb (7 examples). All 14 executed against a real local Mongo/Redis and passed. Each behavior also has a companion check that reconstructs the pre-fix code path in isolation to confirm the new assertions would have failed against the old bug (discriminating power), e.g.:

• old fall-through logic returns true after a real Timeout::Error with the lock file still present — spec expecting false + lock gone would fail against it.
• old write_lock (flock-release only) leaves the lock file on disk after the block raises — spec expecting it gone would fail against it.
• old 3-retry rescue triggers a 2nd extract_archive call — expect(...).to receive(:extract_archive).once raises MockExpectationError against it.
• old @@data_point reference raises NameError (uninitialized class variable, never assigned anywhere in the codebase) rather than silently applying an invalid timeout — differs from the incident hypothesis but confirms the bug is real and the fix's 28800 readback behavior is correct.

No project-wide test suite/linter run (per repo convention, left to CI).

Not in scope here

• Seed-zip re-upload validation — already covered by Enhance InitializeAnalysis resilience for corrupt seed.zip files #841/Fail fast on corrupt seed.zip: validate at upload, no futile retries, terminal 'failed' state (#841) #844 (different code path).
• The 85 corrupted seed-zip batches from this incident are being resubmitted separately once this fix (and Enhance InitializeAnalysis resilience for corrupt seed.zip files #841/Fail fast on corrupt seed.zip: validate at upload, no futile retries, terminal 'failed' state (#841) #844, confirmed not yet deployed to the production image as of this incident) are live.

[brianlball]

Reviewed and verified this locally — the bugs are real and the fix is sound, with one Windows-specific flaw I've fixed in a follow-on commit (7c10e4c) pushed to this branch.

Verification (red-green against unfixed develop)

Checked all three claims directly in develop source (fall-through return true after rescue Timeout::Error, lock file surviving write_lock failure, @@data_point never assigned anywhere), then ran this PR's specs against develop's app code with a local mongo: 10/14 fail on unfixed code, every core claim reproducing. Against this branch's app code: green on Linux CI, 12/14 on Windows — which surfaced the flaw below.

Finding: the cleanup deleted files whose fds were still open

Windows cannot unlink an open file, and FileUtils.rm_f/rm_rf swallow that failure silently:

1. write_lock's rescue ran FileUtils.rm_f lock_file_path before the ensure closed the fd — so on Windows the stale lock survives and the 8h deadlock this PR fixes remains, silently. Desktop/PAT-on-Windows is exactly the deployment write_lock's own comment says it protects ("threadsafe for non-docker deployments, i.e. desktop usage").
2. The corrupt-zip rescue ran FileUtils.rm_rf(analysis_dir) while still inside the write_lock block — the lock fd (inside analysis_dir) is open at that point, so the directory survives on Windows.

Repro: spec/models/dj_run_simulate_data_point_hardening_spec.rb:127 and :173 fail on Windows against 5ab3176.

Fix (7c10e4c)

• write_lock rescue: unlock + close the fd, then rm_f; the ensure only closes if the rescue hasn't already.
• Corrupt-zip rescue: sweep the extracted content but leave the (still-open) lock file for write_lock to delete; raise a typed CorruptAnalysisZip (same message, so the spec assertions hold) and let a new initialize_worker-level rescue remove the emptied analysis_dir after the lock is gone. Dir.rmdir rather than rm_rf there: if another worker has already re-locked and begun re-populating the directory, rmdir fails harmlessly instead of deleting its in-progress files.

All 14 hardening specs now pass on Windows (were 12/14); Linux behavior and spec assertions unchanged.

Heads-up: this PR and #847 touch neighboring hunks in run_simulate_data_point.rb (the @@data_point upload-timeout fix sits inside #847's upload_file rewrite) — whichever merges second will need a small mechanical rebase.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.

Hardens DjJobs::RunSimulateDataPoint and its Resque dispatcher against stale lock/receipt deadlocks, false-success fallthrough after timeouts, and futile retries on deterministically corrupt analysis zips (plus fixes a long-standing @@data_point typo that prevented timeout defaults from being applied).

Changes:

• Add lock/receipt wait-loop safeguards (detect missing lock holder, delete stale locks on real timeout, and fail instead of falling through to success).
• Fail fast on corrupt zip extraction (Zip::Error/Zlib::Error) with cleanup and a typed exception for targeted handling.
• Add/expand regression specs to prevent deadlock reintroduction and to validate the new Resque run_flag dispatch guard.

Reviewed changes

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

Show a summary per file

File	Description
server/app/jobs/dj_jobs/run_simulate_data_point.rb	Implements lock/receipt deadlock hardening, corrupt-zip fail-fast path, and fixes @@data_point timeout assignments.
server/app/jobs/resque_jobs/run_simulate_data_point.rb	Adds analysis.run_flag == false guard to prevent stale queue payloads from dispatching new workers.
server/app/jobs/dj_jobs/urban_opt.rb	Fixes @@data_point typo so workflow timeouts are set on the actual instance analysis.
server/spec/models/dj_run_simulate_data_point_hardening_spec.rb	Adds regression coverage for abandoned locks, timeouts, corrupt zip extraction, and the timeout typo fix.
server/spec/models/resque_run_simulate_data_point_hardening_spec.rb	Adds regression coverage for the new Resque run_flag dispatch guard behavior.

---

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

[brianlball]

Rebased onto develop (post-#847/#849: single CI run per push, no rserve rebuild since this PR doesn't touch docker/R) and pushed two more commits. Summary of what happened and what changed:

Why the docker re-run failed (real regression, not flaky)

The re-run executed the merge ref (this branch + current develop), which was the first time the new run_flag guard met the re-enabled resque_run_simulation_data_point_spec from #847 — and the spec caught a genuine bug: run_flag defaults to false on every new analysis, so the guard couldn't distinguish "ops stopped this analysis" from "nobody has started it yet". Any datapoint submitted directly against a fresh analysis (batch upload + submit_simulation, a real API/PAT flow) was silently skipped (SKIPPING ... run_flag=false in the job log; status_message stayed empty).

Fix (8314b34): treat run_flag == false as "stopped" only when the analysis has a start_time (its jobs exist iff it was started). The hardening spec's stopped-context now creates the Job a genuinely stopped analysis would have, and there's a new regression example for the never-started case.

Copilot review dispositions

• Timeout-path lock deletion (accepted, extended): correct catch — a live-but-slow holder can outlive a single waiter timeout (each holder step gets its own initialize_worker_timeout), and deleting its lock lets a third worker initialize the same analysis_dir concurrently on a fresh inode. Implemented the suggested flock(LOCK_EX|LOCK_NB) probe: the timeout path now only clears locks nobody holds. Bonus: the same probe in the wait loop (two consecutive positives, one poll apart, so a holder's open→flock startup gap can't misread) detects SIGKILL/OOM-killed holders immediately — the incident's actual failure mode, which rescue-based cleanup can never see — instead of after the full 8h.
• @intialize_worker_errs rename (declined): cosmetic; not worth spreading the diff. Can be a standalone cleanup later.
• Corrupt-zip fixture byte-flip (declined): the concern doesn't hold. String#index finds the local file header (it always precedes the central directory), rubyzip 2.3's OutputStream writes no extra field, so the flip lands deterministically in the DEFLATE stream. And the failure direction is backwards: if the fixture ever failed to corrupt, the .once message expectation and the error-text assertions fail loudly — there's no silent pass.

Verification on the rebased branch

• Hardening specs: 15/15 (Windows local + Linux container).
• Full docker-compose stack: resque_run_simulation_data_point_spec 2/2 (the exact CI failure, now green) and dj_run_simulation_data_point_spec 7/7.

🤖 Generated with Claude Code