Harden RunSimulateDataPoint worker against permanent lock deadlock and stale-job re-dispatch#845
Open
anchapin wants to merge 1 commit into
Open
Harden RunSimulateDataPoint worker against permanent lock deadlock and stale-job re-dispatch#845anchapin wants to merge 1 commit into
anchapin wants to merge 1 commit into
Conversation
…d stale-job re-dispatch Root cause of 2026-07-07/08 production incident: 50+ analyses with corrupted seed zips caused `write_lock` (dj_jobs/run_simulate_data_point.rb) to release its flock on failure but never delete the lock file, and never write a receipt. The waiter loop only polled for the receipt file and never rechecked the lock, so every subsequent worker pod for the same analysis blocked forever on `initialize_worker`, waiting out the full 8h `initialize_worker_timeout` and then (due to a second bug) falling through to `return true` anyway -- reporting fake success and letting `perform` proceed to simulate against a directory that was never populated. Fleet-wide this produced ~9,988 permanently-zombied worker pods (99.6% of the tier). Fixes: - `write_lock`: rescue any exception in the protected block, delete the lock file before re-raising (previously only released the flock, leaving a file a future worker could stat as "still locked"). 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 -- the crash/failure signature) and return `false` instead of looping to timeout. On genuine `Timeout::Error`, explicitly delete the stale lock file and return `false` instead of falling through to `return true`. - Add a `rescue ::Zip::Error, ::Zlib::Error` branch ahead of the generic retry rescue in the zip-extraction path: fails fast on deterministic corruption (no futile 3x retry), removes the partial `analysis_dir`. Worker-side counterpart to the web-node fail-fast added for seed.zip validation in #841/#844 (a different, once-per-analysis code path; this is the per-datapoint, per-worker-pod download+extract path). - Fix `@@data_point` -> `@data_point` (uninitialized class variable vs the actual instance variable) in the invalid-timeout fallback branches: 3 occurrences in `dj_jobs/run_simulate_data_point.rb`, 2 more in `dj_jobs/urban_opt.rb` (included into the same class). Bug present since ebc2361 (2019), also confirmed on origin/develop. Previously any invalid timeout value raised NameError instead of applying the 28800s default. - `resque_jobs/run_simulate_data_point.rb#perform`: add a narrow `analysis.run_flag == false` guard (checked ahead of the existing completed-status guard) to skip stale queue payloads for analyses that were explicitly stopped/quarantined via `stop_analysis`/ `soft_stop_analysis`. Deliberately not a blanket `status == 'completed'` check, which would break the admin manual-retry workflow (`requeue`/ `requeue_started` do not reset status before re-enqueueing). Adds `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 executed against a real local Mongo/Redis and passing. Each also has a companion "would fail against the pre-fix code" check to confirm discriminating power.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_lockreleased itsflockon failure but never deleted the lock file, and never wrote a receipt. The waiter loop ininitialize_workeronly 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 fullinitialize_worker_timeout(8h default), and then, due to a second bug, fell through toreturn trueanyway: reporting fake success and lettingperformproceed 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 inensure(minor fd leak).initialize_worker's receipt-wait loop: detectlock_holder_gone(lock file disappears without a receipt appearing) and returnfalseimmediately instead of looping to timeout. On genuineTimeout::Error, explicitly delete the stale lock file and returnfalseinstead of the old fall-throughreturn true.rescue ::Zip::Error, ::Zlib::Errorahead of the generic retry rescue — fail fast on deterministic corruption (no futile 3x retry), remove the partialanalysis_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_pointtypo fix (5 occurrences: 3 indj_jobs/run_simulate_data_point.rb, 2 indj_jobs/urban_opt.rb, which isincluded into the same class): the invalid-timeout fallback branches referenced an uninitialized class variable instead of the instance variable, sinceebc2361b(2019). Confirmed also present onorigin/develop. Previously any invalid timeout value raisedNameErrorinstead of applying the intended 28800s default.resque_jobs/run_simulate_data_point.rb#perform: added a narrowanalysis.run_flag == falseguard (checked ahead of the existing completed-status guard) to skip stale queue payloads for analyses explicitly stopped/quarantined viastop_analysis/soft_stop_analysis. Deliberately not a blanketstatus == 'completed'check — that would break the admin manual-retry workflow, sincerequeue/requeue_starteddon't reset status before re-enqueueing.Testing
Added
server/spec/models/dj_run_simulate_data_point_hardening_spec.rb(7 examples) andserver/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.:trueafter a realTimeout::Errorwith the lock file still present — spec expectingfalse+ lock gone would fail against it.write_lock(flock-release only) leaves the lock file on disk after the block raises — spec expecting it gone would fail against it.extract_archivecall —expect(...).to receive(:extract_archive).onceraisesMockExpectationErroragainst it.@@data_pointreference raisesNameError(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's28800readback behavior is correct.No project-wide test suite/linter run (per repo convention, left to CI).
Not in scope here