Fix busy-spin loops causing test_io_enable_disable flakiness - #642
Open
nllong wants to merge 2 commits into
Open
Fix busy-spin loops causing test_io_enable_disable flakiness#642nllong wants to merge 2 commits into
nllong wants to merge 2 commits into
Conversation
- step_run_process.py: advance()/stop() waited on multiprocessing Event proxies with a tight, unslept while-loop, pegging a full CPU core for the entire duration of every advance() call. Under CPU contention (e.g. constrained CI runners, --scale worker=2) this starves the simulation subprocess and makes it more likely to overrun advance_timeout, corrupting/aborting the run mid-step and producing stale/incorrect point values on the next read - the likely cause of the intermittent test_io_enable_disable failures. Added a short sleep in both loops. - job.py: start_message_loop's _check_messages() called redis_pubsub.get_message() non-blocking in a tight loop, hset()'ing the job status to redis on every spin iteration. Switched to a 1s blocking read so idle jobs stop hammering redis/CPU while waiting on the next message. - Updated the corresponding test double (MockRedisPubSub) to accept the new timeout argument. Verified locally: full unit test suite passes, and test_small_office_osw.py::test_io_enable_disable passes repeatedly (including under artificially CPU-limited worker containers, the scenario most likely to trigger the original race). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
1 participant
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
Split out from #633, where the underlying flaky-test investigation happened.
tests/integration/test_small_office_osw.py::test_io_enable_disablehas been intermittently failing in CI with a wrong output value (e.g. assertingPython Output == 20but getting21.735...), unrelated to any change in that PR. Root-caused this to two busy-spin (tight, unsleptwhileloops) bugs in the worker's job/simulation synchronization code:StepRunProcess._wait_for_event(used byadvance()) andStepRunProcess.stop()wait onmultiprocessing.ManagerEventproxies for the event to be cleared, but the loop had no sleep in that branch — it busy-spins at ~100% CPU for the entire duration of everyadvance()/stop()call.Job._check_messagescalledredis_pubsub.get_message()with the default non-blockingtimeout=0, sostart_message_loopspins as fast as possible while idle,hset()-ing the job status to redis on every iteration.Under CPU contention (constrained CI runners,
--scale worker=2running two of these loops concurrently) this starves the EnergyPlus simulation subprocess of CPU time, making it more likely to overrunadvance_timeoutand abort/corrupt the run mid-step — producing stale or incorrect point values on the next read. This is the most likely explanation for the intermittent failure.Changes
alfalfa_worker/jobs/step_run_process.py: add a shortsleep(0.05)in both busy-spin loops.alfalfa_worker/lib/job.py: use a 1s blockingget_message(timeout=1.0)read instead of a non-blocking poll.tests/worker/lib/mock_redis_pub_sub.py: update the test double to accept the newtimeoutargument (mirrors redis-py'sPubSub.get_messagesemantics).Testing
poetry run pytest): 30 passed.test_io_enable_disablerepeatedly, including with worker containers CPU-limited viadocker update --cpus=1.0(the scenario most likely to trigger the original race) — passed consistently after the fix.pre-commit runpasses on all changed files.Note: I was not able to 100% deterministically reproduce the original race locally even before this fix (it's rare), so this can't be proven to fully eliminate the flakiness — but the busy-spin loops are a real, independently-justified defect (CPU/redis thrashing) and a strong contributing factor given the failure's timing-sensitive nature.