Skip to content

Fix mrun hanging when a worker dies and leaking process pools - #1156

Merged
rkingsbury merged 2 commits into
materialsproject:mainfrom
rkingsbury:fix-mrun-worker-death
Oct 27, 2026
Merged

rkingsbury merged 2 commits into
materialsproject:mainfrom
rkingsbury:fix-mrun-worker-death

Conversation

@rkingsbury

Copy link
Copy Markdown
Collaborator

Summary

Fixes two long-standing bugs in the mrun -n N multiprocessing runner (src/maggma/cli/multiprocessing.py), found while looking into the Python 3.14 CI hang (materialsproject/maggma#1151).

Major changes:

  • fix 1: a dead worker no longer hangs mrun. When a worker crashed or was OOM-killed, AsyncUnorderedMap swallowed the BrokenProcessPool error and the fill task died without ever posting its "done" sentinel, so the consumer waited on the results queue forever. Now any BrokenExecutor, or any exception raised by the source iterator (e.g. a failing cursor), is raised from iteration and propagates out of multi().
    • Per-item executor errors (e.g. an item that can't be pickled) are now logged and skipped, the same way safe_dispatch already handles errors inside process_item. Before, they were silently dropped and never released their back-pressure slot, so after chunk_size of them the run also hung.
    • The fill task now gathers the per-item wrapper tasks instead of the raw futures. That way the done sentinel can't be queued ahead of the last result, and the wrapper tasks stay referenced (this resolves the RUF006 TODO).
    • Sentinel check changed from == to is, so result objects with a custom __eq__ (numpy, pandas) can't break it.
  • fix 2: process pools are always shut down. multi() created a ProcessPoolExecutor and never shut it down. Each leaked pool kept its management thread alive, and forking new pools while those threads were running is a known cause of intermittent deadlocks (e.g. several mrun -n 2 calls in one pytest process). The executor is now shut down in a finally with cancel_futures=True, both on success and on error.

Todos

Checklist

  • Google format doc strings added.
  • Code linted with ruff. (pre-commit run passes on the changed files.)
  • Type annotations included. Check with mypy. (I didn't add annotations, to match the surrounding code.)
  • Tests added for new features/fixes. New tests: a worker that calls os._exit raises BrokenProcessPool, a failing source iterator raises, an unpicklable item is skipped, and multi() shuts down its executor both on success and when update_targets raises. All four time out or fail on main.
  • I have run the tests locally and they passed. tests/cli/test_multiprocessing.py passes on 3.13 (with both fork and forkserver start methods) and on 3.14. The non-Mongo mrun -n 2 tests in test_init.py also pass. I couldn't run the Mongo-backed tests locally because no mongod was available, so CI covers those.

🤖 Generated with Claude Code

claude and others added 2 commits October 27, 2026 14:53
- AsyncUnorderedMap now raises BrokenProcessPool (or any error from the
  source iterator) from iteration instead of waiting forever. Per-item
  executor failures such as unpicklable items are logged and skipped
  like errors inside process_item, so they no longer leak a
  back-pressure slot.
- Gather the per-item wrapper tasks rather than the raw futures, so the
  done sentinel cannot overtake the last result, and keep references to
  them (resolves the RUF006 TODO).
- multi() always shuts down its ProcessPoolExecutor, including on error.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013Pt7k5BrMjWxfc4edxL6mg
@codecov

codecov Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.85714% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.92%. Comparing base (c07ace8) to head (9759b66).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/maggma/cli/multiprocessing.py 92.85% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1156      +/-   ##
==========================================
+ Coverage   69.77%   69.92%   +0.15%     
==========================================
  Files          47       47              
  Lines        4106     4120      +14     
==========================================
+ Hits         2865     2881      +16     
+ Misses       1241     1239       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rkingsbury
rkingsbury merged commit 88cd839 into materialsproject:main Oct 27, 2026
14 checks passed
@rkingsbury
rkingsbury deleted the fix-mrun-worker-death branch October 27, 2026 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants