Repository navigation
Conversation
Close the distributor before waiting for the worker PID pool so a dead round-robin worker cannot leave shutdown waiting forever on its private queue. Surviving workers can finish queued work, including long tests and workers that register late. After every child exits, report remaining queued and in-flight tests through the existing missing-result path without adding a production timeout.
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core, timing-sensitive parallel-test shutdown behavior involving fork/DRb/signal handling that affects every Rails parallel test run, so it warrants final human review despite appearing correct and well-tested.
0 open findings
What changed in this PR
This PR fixes a hang in Active Support's parallel test runner (issue #59037): when a round-robin worker process dies while it still has queued tests, those tests can never be drained, so Server#shutdown's old while @distributor.pending? loop spins forever and the run never reports the unfinished work.
The fix reworks shutdown to be driven by the actual worker PIDs rather than queue state: close the distributor first, block on Process.waitpid for every forked PID (including workers that never registered or crashed early), then drain any tests the dead workers left in their queues and report them (plus still-in-flight tests) through the existing "result not reported" path. This removes the reliance on active_workers?/reap_dead_workers polling and the pre-close pending? drain.
Changes:
- Rework
Server#shutdownto acceptworker_pids, wait on real PIDs via a newwait_for_workers, then drain leftover queued tests from dead workers and report them as unreported. - Simplify
Parallelization#shutdownto delegate PID waiting to the server by passing@worker_pool. - Add regression tests covering queued/in-flight work, early exits, delayed registration, work stealing, and interrupt cancellation, plus a real worker-crash reproduction; update the changelog.
| File | Description |
|---|---|
activesupport/lib/active_support/testing/parallelization/server.rb |
Replaces the pending? drain + wait_for_active_workers/reap_dead_workers with shutdown(worker_pids) that waits on real PIDs and drains dead workers' queues. |
activesupport/lib/active_support/testing/parallelization.rb |
Delegates worker-PID waiting to @queue_server.shutdown(@worker_pool). |
activesupport/test/parallelization/server_test.rb |
Adds unit tests for dead-worker draining, early exit, work stealing, interrupt, and delayed registration, with PID cleanup in setup/teardown. |
activesupport/test/parallelization_test.rb |
Adds an end-to-end worker-crash regression test and updates a stale comment to match the new flow. |
activesupport/CHANGELOG.md |
Documents the fix referencing #59037. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Motivation / Background
Fixes #59037. When a round-robin worker dies, its queued tests can never drain, so shutdown hangs before reporting unfinished work.
Detail
Close the distributor first, wait for the actual worker PIDs (including workers that have not registered yet), then report remaining queued and in-flight tests through the existing missing-result path. Surviving workers can finish their work, and no production timeout is added. This addresses dead workers; #57086 concerns workers that remain alive but get stuck.
Regression tests cover queued/in-flight work, early exits, delayed registration, work stealing, interrupt cancellation, and the real worker-crash reproduction.
Validation
BacktraceCleanerCleanLocationsTest#test_returns_all_clean_locations_(defaults)and#test_returns_all_clean_locations_(:silent): they expect only the test-file location but retain worker frames. Both reproduce on the original baseline. A timing-sensitive CI test failed on the first full run and passed on both baseline/patched focused reruns and the final full run.Checklist